Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for vertical text bounds and alignment #9133

Merged
merged 12 commits into from
Jul 13, 2023

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Jul 12, 2023

Objective

In both Text2d and Bevy UI text because of incorrect text size and alignment calculations if a block of text has empty leading lines then those lines are ignored. Also, depending on the font size when leading empty lines are ignored the same number of lines of text can go missing from the bottom of the text block.

Example (from murtaugh on discord)

use bevy::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .run();
}

fn setup(mut commands: Commands) {
    commands.spawn(Camera2dBundle::default());

    let text = "\nfirst line\nsecond line\nthird line\n";

    commands.spawn(TextBundle {
        text: Text::from_section(
            text.to_string(),
            TextStyle {
                font_size: 60.0,
                color: Color::YELLOW,
                ..Default::default()
            },
        ),
        style: Style {
            position_type: PositionType::Absolute,
            ..Default::default()
        },
        background_color: BackgroundColor(Color::RED),
        ..Default::default()
    });
}

Solution

TextPipeline::queue_text, TextMeasureInfo::compute_size_from_section_texts and GlyphBrush::process_glyphs each have a nearly duplicate section of code that calculates the minimum bounds around a list of text sections.

The first two functions don't apply any rounding, but process_glyphs also floors all the values. It seems like this difference can cause conflicts where the text gets incorrectly shaped.

Also when Bevy computes the text bounds it chooses the smallest possible rect that fits all the glyphs, ignoring white space. The glyphs are then realigned vertically so the first glyph is on the top line. Any empty leading lines are missed.

This PR adds a function compute_text_bounds that replaces the duplicate code, so the text bounds are rounded the same way by each function. Also, since Bevy doesn't use ab_glyph to control vertical alignment, the minimum y bound is just always set to 0 which ensures no leading empty lines will be missed.

There is another problem in that trailing empty lines are also ignored, but that's more difficult to deal with and much less important than the other issues, so I'll leave it for another PR.

fixed_text_align_bounds

Changelog

Added a new function compute_text_bounds to the glyph_brush module that replaces the text size and bounds calculations in
TextPipeline::queue_text, TextMeasureInfo::compute_size_from_section_texts and GlyphBrush::process_glyphs. The text bounds are calculated identically in each function and the minimum y bound is not derived from the glyphs but is always set to 0.

# Objective

`GlobalTransform` after insertion will be updated only on `Transform` or
hierarchy change.

Fixes bevyengine#9075

## Solution

Update `GlobalTransform` after insertion too.

---

## Changelog

- `GlobalTransform` is now updated not only on `Transform` or hierarchy
change, but also on insertion.
…on_texts`

and `GlyphBrush::process_glyphs` each have a section of code that calculates the minimum bounds around a list of text sections.

The first two functions don't apply any rounding, but  `process_glyphs` also floors all the values. It seems like this can cause conflicts where the text gets incorrectly shaped.

Also when Bevy computes the text bounds it chooses the smallest possible rect that fits all the glyphs, ignoring white space. The glyphs are then realigned vertically so the first glyph is on the top line. Leading empty lines are missed off.

This PR adds a function `compute_text_bounds` that replaces the duplicate code, so the text bounds are rounded the same way by each function. Also, since Bevy doen't use ab_glyph to control vertical alignment, the min y bound is just always set to 0 which ensures no leading empty lines will be will missed.

There is another problem in that trailing empty lines are also ignored, but that's more difficult to deal with and much less important than the other issues.
…on_texts`

and `GlyphBrush::process_glyphs` each have a section of code that calculates the minimum bounds around a list of text sections.

The first two functions don't apply any rounding, but  `process_glyphs` also floors all the values. It seems like this can cause conflicts where the text gets incorrectly shaped.

Also when Bevy computes the text bounds it chooses the smallest possible rect that fits all the glyphs, ignoring white space. The glyphs are then realigned vertically so the first glyph is on the top line. Leading empty lines are missed off.

This PR adds a function `compute_text_bounds` that replaces the duplicate code, so the text bounds are rounded the same way by each function. Also, since Bevy doen't use ab_glyph to control vertical alignment, the min y bound is just always set to 0 which ensures no leading empty lines will be will missed.

There is another problem in that trailing empty lines are also ignored, but that's more difficult to deal with and much less important than the other issues.
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets labels Jul 12, 2023
@ickshonpe
Copy link
Contributor Author

This should be added to 0.11.1, it's an annoying bug.

@nicopap nicopap added this to the 0.11.1 milestone Jul 13, 2023
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one adjustment for compute_text_bounds

crates/bevy_text/src/glyph_brush.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice solution.

@cart cart added this pull request to the merge queue Jul 13, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 13, 2023
@cart cart added this pull request to the merge queue Jul 13, 2023
Merged via the queue into bevyengine:main with commit c7ca7dd Jul 13, 2023
@nicopap nicopap mentioned this pull request Jul 18, 2023
cart pushed a commit that referenced this pull request Aug 10, 2023
# Objective

In both Text2d and Bevy UI text because of incorrect text size and
alignment calculations if a block of text has empty leading lines then
those lines are ignored. Also, depending on the font size when leading
empty lines are ignored the same number of lines of text can go missing
from the bottom of the text block.

## Example (from murtaugh on discord)

```rust
use bevy::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .run();
}

fn setup(mut commands: Commands) {
    commands.spawn(Camera2dBundle::default());

    let text = "\nfirst line\nsecond line\nthird line\n";

    commands.spawn(TextBundle {
        text: Text::from_section(
            text.to_string(),
            TextStyle {
                font_size: 60.0,
                color: Color::YELLOW,
                ..Default::default()
            },
        ),
        style: Style {
            position_type: PositionType::Absolute,
            ..Default::default()
        },
        background_color: BackgroundColor(Color::RED),
        ..Default::default()
    });
}

```


![](https://cdn.discordapp.com/attachments/1128294384954257499/1128295142072254525/image.png)

## Solution

`TextPipeline::queue_text`,
`TextMeasureInfo::compute_size_from_section_texts` and
`GlyphBrush::process_glyphs` each have a nearly duplicate section of
code that calculates the minimum bounds around a list of text sections.

The first two functions don't apply any rounding, but `process_glyphs`
also floors all the values. It seems like this difference can cause
conflicts where the text gets incorrectly shaped.

Also when Bevy computes the text bounds it chooses the smallest possible
rect that fits all the glyphs, ignoring white space. The glyphs are then
realigned vertically so the first glyph is on the top line. Any empty
leading lines are missed.

This PR adds a function `compute_text_bounds` that replaces the
duplicate code, so the text bounds are rounded the same way by each
function. Also, since Bevy doesn't use `ab_glyph` to control vertical
alignment, the minimum y bound is just always set to 0 which ensures no
leading empty lines will be missed.

There is another problem in that trailing empty lines are also ignored,
but that's more difficult to deal with and much less important than the
other issues, so I'll leave it for another PR.

<img width="462" alt="fixed_text_align_bounds"
src="https://github.com/bevyengine/bevy/assets/27962798/85e32e2c-d68f-4677-8e87-38e27ade4487">


---

## Changelog

Added a new function `compute_text_bounds` to the `glyph_brush` module
that replaces the text size and bounds calculations in
`TextPipeline::queue_text`,
`TextMeasureInfo::compute_size_from_section_texts` and
`GlyphBrush::process_glyphs`. The text bounds are calculated identically
in each function and the minimum y bound is not derived from the glyphs
but is always set to 0.
@cart cart mentioned this pull request Oct 13, 2023
43 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants